Codecov Report

Base: 80.57% // Head: 80.80% // Increases project coverage by +0.23% 🎉

Coverage data is based on head (854ea65) compared to base (60bbdcc).
Patch coverage: 82.43% of modified lines in pull request are covered.

@@            Coverage Diff             @@
##           master    #4371      +/-   ##
==========================================
+ Coverage   80.57%   80.80%   +0.23%     
==========================================
  Files          28       30       +2     
  Lines        7319     7449     +130     
==========================================
+ Hits         5897     6019     +122     
- Misses       1422     1430       +8     

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Sounds good to me in principle, I'm happy to give any of my contributions to Plots.jl.

I find it hard to say anything about the implementation, there seems to be quite some unrelated edits etc. But if Plots.jl maintainers approve I don't object.

I'm all for integrating UnitfulRecipes.jl into Plots.jl as well! Just make sure this does not blow up TTFP! (Does it even affect it?)

Just make sure this does not blow up TTFP! (Does it even affect it?)

Regarding TTFP:

master (or PR with @require Unitful block commented in src/init.jl)

using Unitful, UnitfulRecipes, Plots

const a = 1u"m/s^2"
v(t) = a * t
x(t) = a / 2 * t^2

main() = begin
  t = (0:.01:100) * u"s"
  plot(x.(t), v.(t), xlabel="position", ylabel="speed") |> display
  return
end

@time main()
# 9.044844 seconds (5.55 M allocations: 289.532 MiB, 1.60% gc time, 96.24% compilation time: 16% of which was recompilation)

PR

using Unitful, Plots

const a = 1u"m/s^2"
v(t) = a * t
x(t) = a / 2 * t^2

main() = begin
  t = (0:.01:100) * u"s"
  plot(x.(t), v.(t), xlabel="position", ylabel="speed") |> display
  return
end

@time main()
# 8.953737 (5.47 M allocations: 285.340 MiB, 1.35% gc time, 96.43% compilation time: 16% of which was recompilation)

If there is any documentation we should also move that into main documentation.

See JuliaPlots/PlotDocs.jl#302 (might need some rework, pending CI).

Concerning the push right question, by "this" do you mean this PR or (this part of) (now) Plots.jl? In the former case I don't think there is any need, the transfer seems quite straightforward. In the latter case, I will probably keep contributing, and it's up to you if you want that to happen by PR or if I should merge my own changes (in conjunction with the others) like before. I'm not intimately familiar with the granularity of GitHub permissions.

Concerning the push right question, by "this" do you mean this PR or (this part of) (now) Plots.jl? In the former case I don't think there is any need, the transfer seems quite straightforward. In the latter case, I will probably keep contributing, and it's up to you if you want that to happen by PR or if I should merge my own changes (in conjunction with the others) like before. I'm not intimately familiar with the granularity of GitHub permissions.

I meant the latter. I think, you know the code best and therefore I would prefer having you being able to make calls on your own, therefore I will send you an invite.

If there is any documentation we should also move that into main documentation.

Also the issues need to be moved.

@briochemc, @gustaphe, @jw3126 if any of you needs/wants push rights to keep working on this just react with a 👍🏻.

Thanks! At the moment I don't use Unitful much, so I probably do not need push access. Should that change, I will come back to you.

The docs build are successful locally and look decent in a browser, but JuliaPlots/PlotDocs.jl#302 needs a new version of Plots with this PR merged.

Note that I've disabled the notebook example in the docs for now, but this can be reworked later.

So I will probably merge this PR when CI is green, unless comments / objections. Following that, the UnitfulRecipes docs should be viewable at https://docs.juliaplots.org/dev/.

Just make sure this does not blow up TTFP! (Does it even affect it?)

Regarding TTFP

That's nice! But I was thinking more about TTFP for non-Unitful users.

There is no impact since loading of the added UnitfulRecipes code in this PR is conditioned to loading Unitful first.
But I should only trust what I see / measure so here are the timings:

master

@time begin
  using Plots
  plot(1:2) |> display
end
 13.199953 seconds (9.41 M allocations: 612.501 MiB, 4.65% gc time, 56.57% compilation time: 12% of which was recompilation)

pr

@time begin
  using Plots
  plot(1:2) |> display
end
 13.254682 seconds (9.41 M allocations: 612.282 MiB, 4.77% gc time, 55.89% compilation time: 12% of which was recompilation)

@jw3126, now that this is released starting from [email protected], some todos:

1. archive https://github.com/jw3126/UnitfulRecipes.jl;
2. (maybe add julia upper bound as suggested in https://discourse.julialang.org/t/how-do-i-deprecate-a-package/14129/2);
3. add deprecation note in https://github.com/jw3126/UnitfulRecipes.jl/blob/master/README.md;
4. transfer opened issues to Plots;
5. (don't know if possible: transfer opened PRs to Plots).

Thanks all for the comments and suggestions.

The new docs are available at https://docs.juliaplots.org/stable/unitfulrecipes/unitfulrecipes/.